-
Notifications
You must be signed in to change notification settings - Fork 14.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove default owner #27541
Remove default owner #27541
Conversation
+1, don't know the implications, but yes please some code cleanups :-) |
@@ -524,9 +524,6 @@ username = | |||
password = | |||
|
|||
[operators] | |||
# The default owner assigned to each new operator, unless | |||
# provided explicitly or passed via ``default_args`` | |||
default_owner = airflow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What will happen to users who customized their airflow.cfg with
default_owner = something
This will no longer work for this... so isn't this a breaking change that we can do only in Airflow 3?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that's correct. Not sure how to best deal with that, should I make a 2nd PR that deprecates the config which can be included with Airflow 2.5?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we deprecating this at all? I actually like the proposal of not using "airflow" as the default, but I don't see why that means we have to also deprecate the ability for folks to set a default owner at the config level at all? Perhaps they have a default that makes sense for their workflow or organizational structure.
IMHO, this config option should just no longer be a "mandatory" value, but still available to use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did some research within Astronomer and there's not a single customer setting this option. I figured we might as well remove it for the sake of simplifying Airflow configuration. WDYT?
Either way (remove/change to None
), this should be treated as a breaking change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following the discussion in #27067 (comment) - this seems like a good candidate to remove IF we agree that we can treat "breaking" in a less strict and more "how likely it's going to break other's workflow" way.
Very good example of a case where we could take a risk and classify it as "non-breaking" (even if it is technically a removal).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got a bit lost in that very long message. Are you proposing to discuss the definition of "breaking change"?
In this PR it's clear --> user facing feature, so treat as breaking change, so change in Airflow 3.0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with Jarek that we could maybe convince ourselves that this is not a hard breaking change, but I actually think this feature is somewhat useful and don't see the motivation for getting rid of it.
We can still keep the ability to set that a globally default user, but just make the config no longer mandatory (since it is currently mandatory). That way you can remove it from the newly published configs, but those users who still have this setting in their configs will not have any regressions and that feature will continue to work.
24967e0
to
4df4406
Compare
Discussion about potential approach we might take for similar cases started in devlist: https://lists.apache.org/thread/1by8ko8jrrp1xwxt5781bwn2tokxjodl |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions. |
I suggest removing the default owner config (introduced 7 years ago in #1318), and defaulting
owner
toNone
.In my experience, the default "airflow" owner never reflects a meaningful entity and thus leads to confusion. Defaulting to None means users have to explicitly set an owner, which aligns better with Python's principle "Explicit is better than implicit".
Currently, you cannot set
owner=None
(link). A workaround is setting an empty string. This PR also fixes the UI to display nothing when the owner is None:After
Before (with
owner=""
)There's likely somebody out there that uses default_owner, so this should be treated as a breaking change.
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rst
or{issue_number}.significant.rst
, in newsfragments.